Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the optional space parameter in JSON.stringify #961

Merged
merged 10 commits into from
Dec 17, 2020
Merged

Implement the optional space parameter in JSON.stringify #961

merged 10 commits into from
Dec 17, 2020

Conversation

tofpie
Copy link
Contributor

@tofpie tofpie commented Dec 12, 2020

This Pull Request fixes/closes #346 .

It changes the following:

  • JSON.stringify() now supports space parameter

@codecov
Copy link

codecov bot commented Dec 12, 2020

Codecov Report

Merging #961 (4aec630) into master (8f388d5) will increase coverage by 0.09%.
The diff coverage is 87.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #961      +/-   ##
==========================================
+ Coverage   59.40%   59.49%   +0.09%     
==========================================
  Files         166      166              
  Lines       10717    10752      +35     
==========================================
+ Hits         6366     6397      +31     
- Misses       4351     4355       +4     
Impacted Files Coverage Δ
boa/src/builtins/json/mod.rs 84.07% <87.17%> (+2.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f388d5...4aec630. Read the comment docs.

Copy link
Member

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but needs a fix:
JSON.stringify({a: "b", b: "c"}, undefined, 4) will return {"a":"b","b":"c"} when it should not.
The code relative to replacer needs to not return immediately, the implementation doesn't follow the spec closely, so I'm not sure as to how we should aproach, but it's a test case to add (I'd say to change your tests with {} as the replacer to use undefined instead).

#919 implements ToIntegerOrInfinity, which could be used to more closely follow the spec, but I don't think it's a big problem.

@RageKnify RageKnify added the builtins PRs and Issues related to builtins/intrinsics label Dec 12, 2020
@tofpie
Copy link
Contributor Author

tofpie commented Dec 12, 2020

Thanks for your review and comments. Let's wait for #919 to be merged, so that ToIntegerOrInfinity can be used to follow the spec more closely.

Copy link
Member

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to bother, but I do think we should use undefined in all of these cases, the spec only uses the 2nd argument when it is a function or array, I'd rather we leave it as undefined to be clear that it is not used.

boa/src/builtins/json/tests.rs Outdated Show resolved Hide resolved
boa/src/builtins/json/tests.rs Outdated Show resolved Hide resolved
boa/src/builtins/json/tests.rs Outdated Show resolved Hide resolved
Co-authored-by: João Borges <rageknify@gmail.com>
@RageKnify RageKnify added the blocked Waiting for another code change label Dec 12, 2020
Copy link
Member

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, we'll just wait for #919 to use ToIntegerOrInfinity

@tofpie
Copy link
Contributor Author

tofpie commented Dec 12, 2020

I have run the test space-string-object.js. It passes on this branch but it also passes on master. I have used the following command:

cargo run --release --bin boa_tester -- run -s test/built-ins/JSON/stringify/space-string-object.js -vv

It is probably a dumb question, and I might have missed something, but why is it pass on master?

@RageKnify
Copy link
Member

RageKnify commented Dec 12, 2020

That test only checks if pairs of invocations return the same result, it never checks if the results follow the spec. So we get:

>> JSON.stringify(obj, null, 'xxx')
"{"a1":{"b1":[1,2,3,4],"b2":{"c1":1,"c2":2}},"a2":"a2"}"
>> JSON.stringify(obj, null, new String('xxx'))
"{"a1":{"b1":[1,2,3,4],"b2":{"c1":1,"c2":2}},"a2":"a2"}"

Being equal and we pass the test.

edit: space-number.js is an example that should failt in master and pass in your branch.

@tofpie
Copy link
Contributor Author

tofpie commented Dec 12, 2020

Thanks for the explanation. I have run space-number.js and also space-string.js on master, and they are both passed.

@RageKnify
Copy link
Member

I have run space-number.js and also space-string.js on master, and they are both passed.

My bad for not reading the test carefully, just like the other one, it only checks if the results match, since we never use the arguments it passes the test.

@tofpie
Copy link
Contributor Author

tofpie commented Dec 12, 2020

It is right for space-number.js, but space-string.js makes an actual comparison with the expected indented string in the second assert. It should probably be failed on master.

@tofpie
Copy link
Contributor Author

tofpie commented Dec 12, 2020

I finally found why the tests are passed on master on my computer. It seems that there is a bug in the parsing of single line comments that occurs when the files use CRLF line endings. The end of line is not detected, so the single line comment extends until the end of the file.

In cursor.rs when a CR is detected, the following LF is swallowed, and only the CR is returned.

match byte {
Some(b'\r') => {
// Try to take a newline if it's next, for windows "\r\n" newlines
// Otherwise, treat as a Mac OS9 bare '\r' newline
if self.peek()? == Some(b'\n') {
let _ = self.iter.next_byte();
}
self.next_line();
}

But the tokenizer for single line comments expects a LF to end a comment, and not a CR.

while let Some(ch) = cursor.peek()? {
if ch == b'\n' {
break;
} else {
// Consume char.
cursor.next_byte()?.expect("Comment character vansihed");
}
}

As I work on Windows, and the test262 repository is cloned with CRLF endings on Windows, so all tests starting with single line comments are passed, as they are considered empty.

Fixing cursor.rs so that it always returns LF for end of lines would probably be the best solution, but it is also possible to fix the single line comment parser so that it detects both CR and LF as line endings. What do you think?

@Razican
Copy link
Member

Razican commented Dec 14, 2020

Fixing cursor.rs so that it always returns LF for end of lines would probably be the best solution, but it is also possible to fix the single line comment parser so that it detects both CR and LF as line endings. What do you think?

I have merged the PR, so a rebase should fix that.

Test262 conformance changes:

Test result master count PR count difference
Total 78,460 78,460 0
Passed 19,374 19,376 +2
Ignored 15,559 15,559 0
Failed 43,527 43,525 -2
Panics 465 465 0
Conformance 24.69 24.70 +0.00%

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, It seems it follows the spec correctly, even though it's not following it step by step exactly. Still, I think it makes sense to do it this way, since the result is the same and it might be a bit more optimized.

One less panic, 3 new extra tests passing, seems nice :)

@Razican Razican added this to the v0.11.0 milestone Dec 15, 2020
@Razican Razican removed the blocked Waiting for another code change label Dec 15, 2020
@tofpie
Copy link
Contributor Author

tofpie commented Dec 15, 2020

In fact, using to_integer_or_infinity would convert a boolean to 1, and so generate an indentation with one space, and that does not follow the spec. Maybe it should stay as it is.

@RageKnify
Copy link
Member

In fact, using to_integer_or_infinity would convert a boolean to 1, and so generate an indentation with one space, and that does not follow the spec. Maybe it should stay as it is.

I don't think that would be the case @tofpie the spec only enters step 6. if the type is Number and that wouldn't be the case for booleans.

@tofpie
Copy link
Contributor Author

tofpie commented Dec 15, 2020

Thanks for the clarification. I understand now. Sorry I did not get it earlier.
I will propose something that follows the spec more closely.

Copy link
Member

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'll let @Razican have a final look at the code

@Razican
Copy link
Member

Razican commented Dec 17, 2020

Looks good to me :) thanks!

@Razican Razican merged commit 751a037 into boa-dev:master Dec 17, 2020
@tofpie tofpie deleted the fix-346 branch December 17, 2020 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the optional space parameter in JSON.stringify( value[, replacer [, space] ] )
3 participants